ci(payjoin): isolate funded testnet fixture - #2590

Claude finished @ethicnology's task in 4m 15s —— View job

Code Review: PR #2590 — ci(payjoin): isolate funded testnet fixture

• Gather context and read the PR diff
• Review changes for correctness bugs, security issues, and regressions
• Post review summary

(Note: I can't submit formal inline review comments, so findings are listed here with file/line references.)

Summary

The change gates the funded testnet Payjoin fixture behind RUN_FUNDED_PAYJOIN_TEST, which the workflow sets to true only for PRs targeting main or manual workflow_dispatch runs. The core logic is correct — no bugs or security issues found. Two minor observations below.

Verified correct:

• ${{ github.base_ref == 'main' || github.event_name == 'workflow_dispatch' }} (.github/workflows/analyze_and_test.yml:179) — github.base_ref is the bare target-branch name on pull_request events and empty on workflow_dispatch, so both clauses behave as intended. The boolean renders as the literal string true/false, matching the Dart check == 'true' at integration_test/payjoin_test.dart:107.
• Fork PRs to main get RUN_FUNDED_PAYJOIN_TEST=true but no secrets, so the existing non-empty-mnemonic checks still skip the group — the fork-safety behavior documented in the old comment is preserved.
• workflow_dispatch requires write access and runs only on branches in the base repo, so exposing secrets to dispatch runs is not a privilege escalation over what PR authors already have.
• coins_test.dart also uses TEST_ALICE_MNEMONIC but only prepares PSBTs (never broadcasts), so leaving it ungated doesn't reintroduce the RBF-conflict problem this PR fixes.

Minor observations

1. Residual race: shared-wallet access is reduced, not serialized — .github/workflows/analyze_and_test.yml:31-33

The concurrency group is ${{ github.workflow }}-${{ github.ref }}, which only cancels superseded runs of the same ref. Two simultaneous PRs to main, or a manual dispatch running alongside a main PR, will still both run the funded fixture against the same shared wallets and can hit the same RBF conflicts. Given that main PRs are rare release gates this is likely an acceptable trade-off, but if it ever bites, a dedicated non-cancelling concurrency group on the integration job (keyed on the funded-test condition) would fully serialize access.

2. Local runs with --dart-define mnemonics now silently skip — integration_test/payjoin_test.dart:90-107

The mnemonics have a dual read path (String.fromEnvironment dart-define falling back to Platform.environment), but RUN_FUNDED_PAYJOIN_TEST is read only from Platform.environment. A developer who previously ran the funded fixture locally via --dart-define=TEST_ALICE_MNEMONIC=... will now get a silent skip, and the skip message (payjoin_test.dart:108-109) only mentions CI paths — it doesn't say that exporting RUN_FUNDED_PAYJOIN_TEST=true is the local opt-in. Consider adding a matching String.fromEnvironment('RUN_FUNDED_PAYJOIN_TEST') fallback and/or mentioning the env var in the skip reason. Fix this →